Skip to content

Conversation

@maxisbey
Copy link
Contributor

Add strict-no-cover from pydantic to the CI pipeline. This tool identifies # pragma: no cover comments on lines that are actually covered by tests, helping keep coverage pragmas accurate.

Changes

  • Add strict-no-cover as dev dependency (installed from git)
  • Add pragma: lax no cover to coverage exclude_lines for partial coverage scenarios
  • Add CI step to run strict-no-cover after coverage report (skips Python 3.14 since the package doesn't support it yet)

How it works

  • # pragma: no cover - errors if any marked lines are actually covered
  • # pragma: lax no cover - permits partial coverage without erroring (useful for flaky or inconsistently covered code)

@maxisbey maxisbey force-pushed the add-strict-no-cover branch 2 times, most recently from 78d6633 to edefce3 Compare January 17, 2026 09:37
@maxisbey maxisbey requested a review from Kludex January 17, 2026 09:37
uv run --frozen --no-sync coverage report
- name: Check for unnecessary no cover pragmas
if: runner.os != 'Windows'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kludex without this all the windows pipelines fail to even run, seems like a strict-no-cover bug? Example failure from previous CI run: https://github.com/modelcontextprotocol/python-sdk/actions/runs/21070443439/job/60598385217

@Kludex
Copy link
Member

Kludex commented Jan 17, 2026

Is this working already? All the pragmas are actually being used?

@maxisbey
Copy link
Contributor Author

Is this working already? All the pragmas are actually being used?

not sure, will test

@maxisbey
Copy link
Contributor Author

@Kludex Found two issues that prevented strict-no-cover from actually detecting anything:

1. coverage >= 7.13 breaks strict-no-cover

Coverage 7.13.0 changed analysis_from_file_reporter to filter executed lines with & statements. Since statements excludes lines matching exclude_lines patterns, executed_lines in the JSON report can never contain excluded lines. strict-no-cover checks excluded_lines ∩ executed_lines, so the intersection is always empty — the tool silently passes regardless of unnecessary pragmas.

Fix: pin coverage[toml]>=7.10.7,<7.13.

2. relative_files = true breaks strict-no-cover

strict-no-cover creates a temp rcfile containing only [tool.coverage.report] and passes it via --rcfile=<temp> to coverage json. This overrides the entire project config, including [tool.coverage.run] settings like relative_files = true. Without that setting, coverage cannot match the relative paths stored in .coverage to the source files, resulting in empty executed_lines for all files.

Fix: remove relative_files = true from [tool.coverage.run].


I also added a test # pragma: no cover on a covered line (src/mcp/shared/exceptions.py:18) to verify the CI step actually fails. Once confirmed, that pragma should be removed.

AI Disclaimer

Add strict-no-cover from pydantic to CI pipeline. This tool identifies
`# pragma: no cover` comments on lines that are actually covered by
tests, helping keep coverage pragmas accurate.

Changes:
- Add strict-no-cover as dev dependency (installed from git)
- Add `pragma: lax no cover` to coverage exclude_lines for partial coverage
- Add CI step to run strict-no-cover after coverage report (Linux only due to Windows bug in the tool)
Two issues prevented strict-no-cover from detecting unnecessary pragmas:

1. Coverage 7.13.x changed analysis_from_file_reporter to filter executed
   lines with '& statements', which removes excluded lines from the
   executed set. This breaks strict-no-cover's detection (it checks the
   intersection of excluded_lines and executed_lines). Pin to <7.13.

2. relative_files = true causes coverage to store relative paths, but
   strict-no-cover creates a temp rcfile without [tool.coverage.run]
   settings. Without relative_files in that temp config, coverage can't
   match paths, resulting in empty executed_lines.

Also adds a test pragma on a covered line to verify the check fails.
@maxisbey maxisbey force-pushed the add-strict-no-cover branch 2 times, most recently from 464a9ff to e942316 Compare January 22, 2026 17:48
Remove ~100 unnecessary '# pragma: no cover' annotations from 30 test
files. These lines are actually covered by tests, as detected by
strict-no-cover in CI.
Restore # pragma: no cover and # pragma: no branch on lines that are
not actually covered in CI (platform-dependent, branch coverage, or
unreachable code paths).
…laky tests

- Remove unnecessary '# pragma: no cover' from 15 src/ files where
  lines are confirmed covered by CI
- Switch flaky test pragmas to '# pragma: lax no cover' for lines that
  are sometimes covered and sometimes not across CI runs
…d ones

- Remove ~80 unnecessary '# pragma: no cover' from src/ files where
  lines are confirmed covered by CI
- Restore pragmas with correct type (no cover vs no branch) for lines
  with missing coverage or branch coverage in batch 1 files
Remove or adjust coverage pragmas in src/ files that were flagged
by strict-no-cover as covering already-tested code.

Changes:
- Remove pragmas from lines/blocks that are fully covered by tests
- Change '# pragma: no cover' to '# pragma: no branch' on lines
  where the condition itself is executed but one branch isn't taken
- Move pragmas from except clause headers to their uncovered bodies
  where the except IS matched but the handler body isn't reached
- Add targeted pragmas to genuinely uncovered code paths (error
  handlers, rarely-hit branches, cleanup paths)

Files modified: 16 src/ files across cli, client, server, and shared
@claude
Copy link

claude bot commented Jan 22, 2026

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

Replace incorrect pragma types with the appropriate ones:
- Use 'lax no cover' for covered lines with uncovered bodies (excluded
  from coverage but not checked by strict-no-cover)
- Use 'no branch' for branches where target lines are covered but the
  branch path itself is not taken
- Keep 'no cover' only for genuinely uncovered code paths
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants